Fix ARMv6 fiber asm: gate Cortex-M0 branch on __ARM_ARCH_6M__#22751
Fix ARMv6 fiber asm: gate Cortex-M0 branch on __ARM_ARCH_6M__#22751andypost wants to merge 1 commit into
Conversation
|
Hit it when upgrading to alpha2 for Alpinelinux https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/105341 |
|
Thanks for tracking this down. As the author of GH-22584, I confirmed the root cause. The Cortex-M0 fallback must be selected specifically with The required changes are:
Your current patch implements these changes correctly. I also verified that:
My commit is here: From my side, GH-22751 is correct and ready for review. |
| git restore save_xmm_x86_64_ms_masm.asm # added in GH-18352, not an upstream boost.context file | ||
|
|
||
| # patch customized files | ||
| git apply -v ../../.github/scripts/download-bundled/boost-context.patch |
There was a problem hiding this comment.
It would be good to name the patch more precisely or even use sed here to make sure __ARM_ARCH >= 7 is correctly replaced even if more #if is added in future upstream sync.
There was a problem hiding this comment.
renamed to boost-context.arm.patch
The Boost.Context 1.91.0 sync (phpGH-22584) pulled in boostorg/context#325, "fix cortex-m0 support to aapcs_elf_gas target", which adds a fallback branch to make_fcontext/jump_fcontext for the Thumb-1 encoding limits of ARMv6-M. The branch is correct for Cortex-M0 but is gated on __ARM_ARCH >= 7, which is also false for classic ARMv6 in ARM state (Alpine armhf, Raspberry Pi 1/Zero) -- targets that can encode every instruction in the >= 7 branch and need none of the workaround. Two consequences. make_fcontext took `ldr a2, =finish', materialising the absolute address of finish through a literal pool in .text and emitting R_ARM_ABS32 against .text. In a PIE link that becomes an R_ARM_RELATIVE dynamic relocation inside the read-only text segment, tagging the binary DT_TEXTREL. musl supports DT_TEXTREL only for DSOs it maps itself; the main executable is kernel-mapped and handled by kernel_mapped_dso(), which never inspects it, so ldso writes to a read-only page and dies with SIGSEGV in do_relocs() before main(). Every SAPI, every invocation, no output. glibc masks this by mprotecting the segment writable, which is why only musl builds noticed. Separately, jump_fcontext -- the hot path on every fiber switch -- grew from 13 to 43 instructions on all ARMv6 builds, glibc included. Gate on __ARM_ARCH_6M__ instead, exactly the target php#325 names. The result emits byte-identical .text to 8.6.0alpha1 on every ARM target except Cortex-M0, where it emits byte-identical .text to current master: each target gets back the code it had before the regression, and M0 keeps what php#325 added. armv7 and arm64 are unaffected either way. Zend/asm is vendored and verified by the Verify Bundled Files workflow, so carry the same diff in .github/scripts/download-bundled and re-apply it from the sync script, as uriparser already does.
d1a2d49 to
6142cf9
Compare
Description
Problem
Since the Boost.Context 1.91.0 sync (9cefeea, GH-22584), every PHP binary
built for ARMv6 as PIE on musl segfaults before
main()— every invocation,every SAPI, no output. First shipped in 8.6.0alpha2; alpha1 is fine. Found on
Alpine Linux armhf (
armv6-alpine-linux-musleabihf,-march=armv6kz).A second, silent regression rides along:
jump_fcontext— the hot path on everyfiber switch — grew from 13 to 43 instructions on all ARMv6 builds, glibc
included.
Root cause
The 1.91.0 sync pulled in boostorg/context#325, titled "fix cortex-m0
support to aapcs_elf_gas target". Its author states the motivation plainly:
The branch works around Thumb-1 encoding limits on ARMv6-M, where
push {r8-r11}andbicwith an immediate do not exist. That is legitimate.The bug is the gate:
__ARM_ARCH >= 7is false for Cortex-M0 — but it is also false for classicARMv6 in ARM state: Alpine armhf, Raspberry Pi 1/Zero. Those targets can
encode every instruction in the
>= 7branch (verified down to ARMv5TE) andwere never the intended audience. They now get Thumb-1-shaped code they cannot
benefit from. Two consequences follow.
1. DT_TEXTREL → startup SIGSEGV on musl.
ldr a2, =finishmaterialises the absolute address offinishthrough aliteral pool in
.text, emittingR_ARM_ABS32against.text. In a PIE linkthat becomes an
R_ARM_RELATIVEdynamic relocation inside the read-only textsegment, tagging the binary
DT_TEXTREL:musl supports
DT_TEXTRELonly for DSOs it maps itself —map_library()mprotects the mapping RWX before relocating (
ldso/dynlink.c, added 2011 incommit 9f17413c "textrel support, cheap and ugly"). The main
executable is mapped by the kernel and handled by
kernel_mapped_dso(), whichnever inspects
DT_TEXTREL. So ldso writes into a read-only page and dies:No fiber ever runs — the loader crashes relocating the binary. glibc masks this
by mprotecting the segment writable and restoring it (
elf/dl-reloc.c), whichis why Raspbian/Debian armv6 never noticed.
Upstream musl considers such binaries defective. Rich Felker, 2020:
Szabolcs Nagy, same thread:
Worth noting the non-crashing case is arguably worse: where musl does handle a
textrel (a shared
libphp.sovia--enable-embed=shared, apache2handler), itleaves the mapping permanently RWX and never restores it, unlike glibc.
So the same defect silently disables W^X there instead of failing loudly.
2.
jump_fcontext3.3× instruction blowup.Same mis-gate, no literal pool, so no crash — just one burst
push {r0,r4-r11,lr}/pop {r3,r4-r11,lr}replaced by elevenstrs and tenldrs, on every fiber switch, on every ARMv6 build.Fix
Gate on
__ARM_ARCH_6M__— precisely the target #325 names:Five occurrences in
make_arm_aapcs_elf_gas.S, two injump_arm_aapcs_elf_gas.S.Cortex-M0 keeps its fallback branch verbatim — including
ldr a2, =finish— sono
.align 2workaround is needed. Classic ARMv6 takesadr a2, finish, asARMv7 does and as every pre-1.91 Boost.Context release did.
Verification
This change introduces no new codegen on any target. Comparing
.textbytesacross three trees — alpha1 (pre-1.91, pre-regression), current master, and this
fix — cross-assembled with clang:
In one sentence: byte-identical to 8.6.0alpha1 on every ARM target except
Cortex-M0, where it is byte-identical to current master. Every affected target
gets back exactly the code it shipped before the regression; Cortex-M0 keeps
exactly what #325 added. Nothing new is invented anywhere.
Instruction counts at armv6kz (
make_fcontext/jump_fcontext):Other architectures are untouched. Scanning every
Zend/asmELF file withclang for x86_64, i386, aarch64, riscv64, ppc64le, s390x and loongarch64 finds
no absolute relocation in
.textanywhere.make_arm_aapcs_elf_gas.Son ARMv6is the only DT_TEXTREL source in the whole tree; aarch64 uses
{make,jump}_arm64_aapcs_elf_gas.S, which this patch does not touch.On Alpine edge armhf (musl, PIE):
./sapi/cli/php -v→Segmentation fault(exit 139, no output);readelf -dshowsTEXTRELand oneR_ARM_RELATIVEinside the text segmentpointing at the
make_fcontextliteral pool.TEXTRELgone,php -vprints the banner, and aFiberstart/suspend/resume/getReturn smoke test passes, confirming both
make_fcontextandjump_fcontextstill work.Alternatives considered
Two smaller changes exist. Neither is more conservative:
ldr a2, =finish→adr a2, finishclears the TEXTREL, but fails toassemble on Cortex-M0 (
error: misaligned pc-relative fixup value—finishlands 2-byte aligned in Thumb-1), i.e. it breaks the exact platformAdd schema default/fixed value support in DOM #325 exists for.
adrplus.align 2beforefinish:is the smallest safeform, but still leaves the
jump_fcontextregression.adr/ldr(one line) fixes thecrash and keeps Cortex-M0 byte-identical, but leaves
jump_fcontextat 43instructions and produces a
make_fcontextmatching neither alpha1 normaster — a hybrid that has never shipped in any PHP or Boost release.
The seven hunks here are seven copies of one mechanical substitution, and they
are what restores already-shipped code on every affected target. A smaller diff
here buys a larger behavioural delta.
Affected
(musl → Alpine armhf and other musl arm32 distros). Crash.
develop/masterand the 1.91.0 release carry the same defect;no upstream issue exists for it yet (searched: textrel, musl, alpine, PIE,
armhf, armv6).
php-src PR: required extra files
Zend/asm/**is vendored and CI-enforced..github/workflows/verify-bundled-files.ymlre-downloads Boost 1.91.0 and runs
.github/scripts/test-directory-unchanged.sh Zend/asm,which ends in
git diff -a --exit-code HEAD Zend/asm. Any local asm edit turns thatjob red — on the PR itself (its paths filter includes
Zend/asm/**) and on the 01:00nightly cron. The next sync would also silently revert the fix.
The established pattern is uriparser's: a checked-in patch re-applied by the download
script. So the PR needs three things, not one:
Zend/asm/{make,jump}_arm_aapcs_elf_gas.S— the gate fix..github/scripts/download-bundled/boost-context.patch— same diff, repo-root-relativepaths (
a/Zend/asm/...), mirroringuriparser.config.patch..github/scripts/download-bundled/boost-context.sh— append, after itscd Zend/asm:# patch customized files git apply -v ../../.github/scripts/download-bundled/boost-context.patch(
git applyresolves repo-root-relative paths from a subdirectory — verified;uriparser.shrelies on the same behaviour with../../../.)